New telemetry values #41
Conversation
Co-authored-by: Ross Grambo <rossgrambo@microsoft.com>
| allocation_percentage = 0 | ||
| for allocation in evaluation_event.feature.allocation.percentile: | ||
| if ( | ||
| evaluation_event.variant |
There was a problem hiding this comment.
It would be better to check whether evaluation_event.variant is None in advance so that we can shortcircuit before the for loop.
There was a problem hiding this comment.
Is it a possible case that variantAssignmentReason is not None but variant is None? I cannot imagine it will happen.
There was a problem hiding this comment.
@zhiyuanliang-ms, I've made some changes already to this in my already existing PR that has some updates. #45
|
|
||
| # DefaultWhenEnabled | ||
| if evaluation_event.feature.allocation and evaluation_event.feature.allocation.default_when_enabled: | ||
| event["DefaultWhenEnabled"] = evaluation_event.feature.allocation.default_when_enabled |
There was a problem hiding this comment.
Just want to call out that it is possible that variant assignment reason is defaultWhenEnabled but the assigned variant is empty/none. The reason of this design is that if we have empty allocation configuration, (in .NET) we can consider it as:
{
"allocation": {
"default_when_enabled" : ""
}
}
There was a problem hiding this comment.
I don't whether it is a better choice to have event["DefaultWhenEnabled"] = "" when there is no allocation.default_when_enabled. But it could be another option for us.
| event["VariantAssignmentPercentage"] = str(100 - allocation_percentage) | ||
| elif evaluation_event.reason == VariantAssignmentReason.PERCENTILE: | ||
| if evaluation_event.feature.allocation and evaluation_event.feature.allocation.percentile: | ||
| allocation_percentage = 0 |
There was a problem hiding this comment.
Can you move this line before the above if statement to make it consistent with line 70
There was a problem hiding this comment.
See my latest PR. I already made these changes.
| for allocation in evaluation_event.feature.allocation.percentile: | ||
| if ( | ||
| evaluation_event.variant | ||
| and allocation.variant == evaluation_event.variant.name |
There was a problem hiding this comment.
@rossgrambo @mrm9084 Can you confirm whether this line is by design please?
There was a problem hiding this comment.
This is incorrect- it's updated in https://github.com/microsoft/FeatureManagement-Python/pull/45/files#diff-e599fecd01f487185b6b19104e39d7a20d310657896ddeee742af944b129945a
Description
Adds VariantAllocationPercentage, DefaultWhenEnabled, and AllocationId to telemetry.
Contribution checklist:
General Guidelines and Best Practices